Update Site to generate default.md based on template #2308

WillCWX · 2023-06-14T13:32:22Z

What is the purpose of this pull request?

edited to follow PR template more accurately

Attempts to:
Resolve #2293

Overview of changes:

Feature / Behaviour Changes:

markbind init --convert
- On an empty folder now produces a website that is exactly the same as markbind init
  - This also means no more about.md as it does not appear in template/default
  - The icon is also changed into Your Logo as it is in default.md
- No longer adds 404 as a navigation link, 404.html is also now displayed exactly the same as in markbind init
- Is now able to be used with -t minimal and has default.md be exactly the same as a minimal template
- generated site.json is now more readable with indentations
- footer and siteNav nunjucks variables no longer replace their defaults and instead adds to their defaults.
markbind init
- default.md is now generated by Site.generateDefaultLayout rather than copied over

Code Maintenance Changes:

Updating default.md layout:
- As default.md should no longer exist, update the respective template .njk nunjucks files instead.
- New TemplateConfig.ts contains template configurations for both default and minimal.
  - njkFile: string dictates which nunjucks file to use.
  - njkSubs: NjkVars[] allows for using nunjucks to copy existing files to incorporate into the layout default.md
  - layoutObjs: SiteConfigPage[] allows for edits to config
  - hasAutoSiteNav: boolean toggles whether Site.buildSiteNav is called during a conversion
  - siteNavIgnore: string[] contains files that Site.buildSiteNav will ignore when making links

Anything you'd like to highlight/discuss:

I have no idea whether this was worth it.
The scope of markbind init --convert is broad and its functions could potentially be used for easy migrations / upgrades.
However, this increase in scope may also unduly burden the Site class as it is the only class currently able to inherently do everything needed to convert. Also Site/index.ts got even longer...

TemplateConfig.ts may not be needed if we decide that templates should be left with just default and minimal as it is.

Testing instructions:
npm run test passes. The functional tests for test_site_templates have not changed but should still pass because the generated default.md for both minimal and default is exactly the same.

Fast-ish Manual Tests:

Copy the non-markbind-site from the conversion tests
npm link with this branch
markbind init --convert the copied non-markbind-site folders
markbind serve to see that they look markbind init but with texts different in the front page and additional / inherited links at the side. Additional texts to footer should exist if _Footer.md is present.

Proposed commit message: (wrap lines at 72 characters)
Update Site to generate default layouts

Checklist: ☑️

Updated the documentation for feature additions and enhancements
Added tests for bug fixes or features
Linked all related issues
No unrelated changes

siteConvertLayout is not able to directly replicate default.md. This results in changes in default.md not being reflected in siteConvertLayout and vice versa. Furthermore, this makes `markdown init -c` on an empty folder unable to be the exact same as `markdown init`. Let's make siteConvertLayout able to clone default.md For siteConvertLayout to clone default.md while still having the customizability of being a default.md file, siteConvertLayout must have default values embedded. Hence, defaultSiteNav is added as defaultFooter already exists. For Site to generate default.md using siteConvertLayout and its existing functions, Site must know what the template requires. Hence, we need to have a TemplateConfig that Site can use to generate the layout or any possible template layouts. Rather than having Template be referenced in Site or vice versa, we will have the command init.js transfer the TemplateConfig from the promise of template.init() to Site.convert or Site.generateDefaultLayout

Some renaming of variables also occured

lhw-1

Good work and thanks for the PR @WillCWX! The changes look quite promising.

Some preliminary overall comments:

As you mentioned, TemplateConfig.ts might not be necessary. However, it still adds a nice avenue for us to easily keep track of new templates if we end up needing more than just default and minimal.
As mentioned in this comment in Markbind init --convert improvements #1866, we may want to remove default file generation for conversion - for that, we'll want to replace instead of adding to siteNav and footer. But for now, adding instead of replacing should be fine to work with the default files being generated.

Also, your addition of in-line documentations have been noted & are appreciated 😄

packages/core/src/Site/template.ts

packages/core/src/Site/index.ts

packages/cli/src/cmd/init.js

packages/core/src/Site/index.ts

lhw-1

Just a nit on naming:

packages/core/src/Site/index.ts

lhw-1

LGTM - Thanks for the PR @WillCWX!

yucheng11122017

Hi @WillCWX thanks so much for the work! Sorry for the late review as well.
Some comments regarding the fileSubstitutions :)

yucheng11122017 · 2023-07-08T14:17:21Z

packages/core/src/Site/TemplateConfig.ts

+
+type NjkVars = {
+  variableName: string,
+  fileSubstitutes: string[]


Why is an array used here? There should only be one file for each substitution
Changes below shows that the last file will be used anyway.

I used an array so that possible variations of file names that could be used as substitutes.
Kind of like how ['README.md', 'Home.md'] is used in addIndexPage()
(link)

Though the current nunjucks variables only have one file name as a substitute, I thought that using an array may allow a greater range of possible file names. (at the cost of some complexity and readability)

Ah I see where you are coming from. That;s ok then :)
Also could you change the variable name to something more obvious that its just for files? Eg. NjkFileVars

packages/core/src/Site/index.ts

lhw-1 · 2023-07-09T13:57:52Z

Also, very minor comment: do make sure to follow the PR template (e.g. the checklist at the beginning for the purpose of PR)

yucheng11122017

LGTM! Thanks for the work @WillCWX

ang-zeyu

I have no idea whether this was worth it.
The scope of markbind init --convert is broad and its functions could potentially be used for easy migrations / upgrades.
However, this increase in scope may also unduly burden the Site class as it is the only class currently able to inherently do everything needed to convert. Also Site/index.ts got even longer...

I think this is still worth it alone for eliminating siteConvertLayout.njk.

I like the idea of TemplateConfig.ts as well, it sets the ground for adding future templates easily.

this increase in scope may also unduly burden the Site class

As a next step (if you are interested), would it be possible to extract all site initialisation and conversion functionality out of Site?
I can't imagine initialisation needs much stateful logic (to be contained within a Site), I see a few references but don't think they should be un-workable around.

It should help out with #1756 greatly

Just a few nits on the PR itself:

ang-zeyu · 2023-07-11T12:43:50Z

packages/core/src/Site/index.ts

-    config.pages.push(layoutObj);
-    await fs.outputJson(configPath, config);
+  static async writeToSiteConfig(config: SiteConfig, configPath: string, layoutObjs: SiteConfigPage[]) {
+    layoutObjs.forEach(layoutObj => config.pages.push(layoutObj));


Suggested change

layoutObjs.forEach(layoutObj => config.pages.push(layoutObj));

siteConfigPages/pageConfigs.forEach(layoutObj => config.pages.push(layoutObj));

I realise this is the old variable name but I don't think its immediately clear (all the way down from TemplateConfig.ts) where it ends up, would this be more straightforward?

ang-zeyu · 2023-07-11T12:43:55Z

packages/core/src/Site/index.ts

    const configPath = path.join(this.rootPath, SITE_CONFIG_NAME);
    const config = await fs.readJson(configPath);
-    await Site.writeToSiteConfig(config, configPath);
+    await Site.writeToSiteConfig(config, configPath, layoutObjs);
  }

  /**
   * Helper function for addDefaultLayoutToSiteConfig().


Suggested change

* Helper function for addDefaultLayoutToSiteConfig().

* Helper function for addTemplateLayoutSettingsToSiteConfig().

ang-zeyu · 2023-07-11T12:55:52Z

packages/core/src/Site/index.ts

@@ -164,6 +158,10 @@ type DeployOptions = {
  user?: { name: string; email: string; },
 };

+type NunjuckObj = {
+  [key: string]: string | undefined,


Standardisation nit: How about Record<string, any>? We are using that elsewhere for nunjucks variables.

ang-zeyu · 2023-07-11T13:10:17Z

packages/core/src/Site/minimalLayout.njk

@@ -0,0 +1,14 @@
+<header>


Do we still need packages\core\template\minimal\_markbind\layouts\default.md? (and the default one)

No we don't need it anymore, I'll delete it now that we are okay with how each template .njk looks in comparison

Sorry, I need to keep packages\core\template\minimal\_markbind\layouts\default.md to make sure git tracks the empty layouts folder. I couldn't get git to track the empty folder otherwise.

I made default.md empty with a message to edit the .njk file instead.

ang-zeyu · 2023-07-11T13:26:33Z

packages/core/src/Site/TemplateConfig.ts

+  hasAutoSiteNav: true,
+  siteNavIgnore: [
+    'index.md', '404.md',
+    'contents/topic1.md', 'contents/topic2.md', 'contents/topic3a.md', 'contents/topic3b.md',


(not your pr but possible future work)

For a conversion feature, what do you think of including 'contents/topic1.md', 'contents/topic2.md', 'contents/topic3a.md', 'contents/topic3b.md'? (personally only just realised its a little odd we are adding pages when doing conversion)

index.md and 404.md makes some sense but I don't think we have file.exists checks or fallback plans if the site already has these files too.

For a conversion feature, what do you think of including 'contents/topic1.md', 'contents/topic2.md', 'contents/topic3a.md', 'contents/topic3b.md'? (personally only just realised its a little odd we are adding pages when doing conversion)

I think it make sense if we think in terms of

converting markdown documents into a default / minimal / template markbind site

Otherwise the navbar will have broken links to contents/topic1.md etc.
Unless we decide to generate the navbar like how we generate the siteNav which is also a viable option

index.md and 404.md makes some sense but I don't think we have file.exists checks or fallback plans if the site already has these files too.

The files are copied over using the util function copySyncWithOptions in Template.init that allows us to ensure that no file override exists. So the plan is to simply use their index.md etc. files as they are if they exist.

Some thing went wrong in the previous merge...

ang-zeyu · 2023-07-23T13:56:03Z

@WillCWX (cc @damithc) for any purposes I think we can consider this PR "merged".

As site/index.ts was recently reverted #2333 to a js file, I suggest leaving the actual merging until that is fixed as there's quite a number of changes to this file here. (After which, rollback the merge commit here including that revert and replay only the code review's changes on top)

ang-zeyu · 2023-07-23T14:11:30Z

Sorry as I do not have time currently to rereview this PR due to the large diffs from the merge commit including the revert. But if anyone else can pick things up here please feel free. 🙇

WillCWX · 2023-07-24T04:28:50Z

@ang-zeyu , thanks for the heads up and for the review previously

As a next step (if you are interested), would it be possible to extract all site initialisation and conversion functionality out of Site?
I can't imagine initialisation needs much stateful logic (to be contained within a Site), I see a few references but don't think they should be un-workable around.

Would it be better if I extract all Site initialisation and conversion functionality out of Site first (in a different pull request) before coming back here?

This would ensure that

process of fixing markbind deploy error after migrating Site/index.js to TypeScript #2333 would not be affected / easier to parse with less functions
General Refactoring work #1756 would progress
no large diffs from the merge commit
this pull request would be able to be merged independently of markbind deploy error after migrating Site/index.js to TypeScript #2333 as it would edit template.ts instead of Site.js / Site.ts

tlylt · 2023-12-03T08:07:56Z

Hi @WillCWX are you keen to continue working on this? If we can fix the conflicts and also outline the remaining points for discussion, I'm happy to support reviewing this PR and get a closure on this.

WillCWX · 2023-12-21T07:26:56Z

@tlylt

Apologies for the late reply, I would still like to work on this issue but I think the conflicts found in this pull request (and on my branch) are a bit too much. So I would like to close this pull request as it is.

I thinking starting fresh on a new branch/pull request will be easier for me to write new tests for the proposed modified init and convert functions (with codecov added, it gets angry when I move previously untested functions to new files without writing new tests).
It will also be easier to avoid overwriting other people's contributions / creating new bugs as I try to fix the conflicts.

Once again, apologies for the late reply and thanks to everyone for reviewing and helping with this pull request.

tlylt · 2023-12-21T08:13:30Z

@tlylt

Apologies for the late reply, I would still like to work on this issue but I think the conflicts found in this pull request (and on my branch) are a bit too much. So I would like to close this pull request as it is.

I thinking starting fresh on a new branch/pull request will be easier for me to write new tests for the proposed modified init and convert functions (with codecov added, it gets angry when I move previously untested functions to new files without writing new tests).

It will also be easier to avoid overwriting other people's contributions / creating new bugs as I try to fix the conflicts.

Once again, apologies for the late reply and thanks to everyone for reviewing and helping with this pull request.

Sure, please aim to make incremental changes (perhaps across multiple PRs) if possible.

WillCWX added 4 commits June 12, 2023 21:15

Update conversion tests to fit default.md layout

4b432a1

Some renaming of variables also occured

Update functional test

8ce2915

Fix spelling mistakes

05551ab

WillCWX marked this pull request as ready for review June 15, 2023 03:07

tlylt requested a review from lhw-1 June 16, 2023 13:40

lhw-1 suggested changes Jun 16, 2023

View reviewed changes

WillCWX added 2 commits June 20, 2023 00:33

Combine convert and generateTemplateDefault

45d0ead

Merge branch 'master' into 2293-generate-default

b522e26

lhw-1 reviewed Jun 20, 2023

View reviewed changes

packages/core/src/Site/index.ts Outdated Show resolved Hide resolved

Rename generateDefaultLayout to initSite

154dbc1

lhw-1 approved these changes Jun 28, 2023

View reviewed changes

lhw-1 requested a review from a team July 6, 2023 13:21

yucheng11122017 reviewed Jul 8, 2023

View reviewed changes

WillCWX added 2 commits July 9, 2023 17:18

Merge branch 'master' into 2293-generate-default

e593fc9

Rename NjkVars to NjkFileVars

2414fb1

yucheng11122017 approved these changes Jul 10, 2023

View reviewed changes

ang-zeyu reviewed Jul 11, 2023

View reviewed changes

WillCWX added 4 commits July 23, 2023 13:41

Rename variables to make more sense

7795437

Update markbind to v5.0.1

9f9fb59

Merge branch 'master' into 2293-generate-default

d281142

Some thing went wrong in the previous merge...

Fix missing minimal layout folder

0904897

ang-zeyu mentioned this pull request Jul 23, 2023

Migrate Site/index.js to TypeScript #2270

Merged

11 tasks

WillCWX mentioned this pull request Jul 31, 2023

Move and Refactor convert to Template #2349

Merged

11 tasks

tlylt marked this pull request as draft August 14, 2023 11:45

tlylt closed this Dec 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Site to generate default.md based on template #2308

Update Site to generate default.md based on template #2308

WillCWX commented Jun 14, 2023 •

edited

Loading

lhw-1 left a comment •

edited

Loading

lhw-1 left a comment

lhw-1 left a comment

yucheng11122017 left a comment

yucheng11122017 Jul 8, 2023

WillCWX Jul 9, 2023

yucheng11122017 Jul 9, 2023

lhw-1 commented Jul 9, 2023

yucheng11122017 left a comment

ang-zeyu left a comment

ang-zeyu Jul 11, 2023

ang-zeyu Jul 11, 2023

ang-zeyu Jul 11, 2023

ang-zeyu Jul 11, 2023

WillCWX Jul 11, 2023

WillCWX Jul 23, 2023

ang-zeyu Jul 11, 2023

WillCWX Jul 11, 2023

ang-zeyu commented Jul 23, 2023

ang-zeyu commented Jul 23, 2023 •

edited

Loading

WillCWX commented Jul 24, 2023

tlylt commented Dec 3, 2023

WillCWX commented Dec 21, 2023

tlylt commented Dec 21, 2023 •

edited

Loading

	layoutObjs.forEach(layoutObj => config.pages.push(layoutObj));
	siteConfigPages/pageConfigs.forEach(layoutObj => config.pages.push(layoutObj));

	* Helper function for addDefaultLayoutToSiteConfig().
	* Helper function for addTemplateLayoutSettingsToSiteConfig().

Update Site to generate default.md based on template #2308

Update Site to generate default.md based on template #2308

Conversation

WillCWX commented Jun 14, 2023 • edited Loading

lhw-1 left a comment • edited Loading

Choose a reason for hiding this comment

lhw-1 left a comment

Choose a reason for hiding this comment

lhw-1 left a comment

Choose a reason for hiding this comment

yucheng11122017 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhw-1 commented Jul 9, 2023

yucheng11122017 left a comment

Choose a reason for hiding this comment

ang-zeyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ang-zeyu commented Jul 23, 2023

ang-zeyu commented Jul 23, 2023 • edited Loading

WillCWX commented Jul 24, 2023

tlylt commented Dec 3, 2023

WillCWX commented Dec 21, 2023

tlylt commented Dec 21, 2023 • edited Loading

WillCWX commented Jun 14, 2023 •

edited

Loading

lhw-1 left a comment •

edited

Loading

ang-zeyu commented Jul 23, 2023 •

edited

Loading

tlylt commented Dec 21, 2023 •

edited

Loading